Add specific error handling for client connect #64
Conversation
There was a problem hiding this comment.
Hey @SeanChinJunKai, thanks for the PR. Could you please add a test for your change?
|
Hi @e5l, I will be adding unit tests, was taking some time to learn more about cancellation exceptions |
|
Hi @e5l, I have added one unit test to test for non-CancellationException. Could I check how CancellationException will be thrown from within the connect function? Based on CancellationException documents: Thrown by cancellable suspending functions if the coroutine is cancelled while it is suspended. It indicates normal cancellation of a coroutine. I am not too sure if the CancellationException will be thrown in the connect function |
SeanChinJunKai
changed the title
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt
Outdated
Show resolved
Hide resolved
| @Test | ||
| fun `should reject due to non cancellation exception`() = runTest { | ||
| var closed = false | ||
| val clientTransport = object : AbstractTransport() { |
There was a problem hiding this comment.
I see clientTransport is very similar to the one in the previous test. should we create a test utility?
There was a problem hiding this comment.
i will look into this
There was a problem hiding this comment.
Hi @e5l ,I havent had much time to look into it but maybe we can have a separate issue to take a look at all tests?
| @@ -189,6 +191,61 @@ class ClientTest { | |||
| assertTrue(closed) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `should reject due to non cancellation exception`() = runTest { | |||
There was a problem hiding this comment.
Can we add test for the case when a Cancellation exception is thrown?
There was a problem hiding this comment.
@ribhavpahuja can i ask how will cancellation exception be thrown in this case? wanted to clarify before adding a test
|
Hi @e5l, could you review this PR please? I have added test for this change |
…ng' into sean/client-connect-error-handling
|
Hi @e5l, I have reverted the changes not sure why the misc.xml still showing up as a diff |
Resolves #20
Motivation and Context
How Has This Been Tested?
Unit Tested
Breaking Changes
No
Types of changes
Checklist